-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
spec-reporter prints retries #5099
spec-reporter prints retries #5099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello again! 😄
Left some thoughts - I'm excited to see this come in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to include retry on same line(retry x1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure if you're ready to re-request review, but just in case 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback on the implementation.
On top of that: The bin/mocha.js
has an unrelated permission change.
} else { | ||
fmt = | ||
indent() + | ||
color('checkmark', ' ' + Base.symbols.ok) + | ||
color('pass', ' %s') + | ||
color(test.speed, ' (%dms)'); | ||
Base.consoleLog(fmt, test.title, test.duration); | ||
Base.consoleLog(fmt, test.title, test.duration, logRetries()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would opt for something like this instead, to keep in the style of the others:
runner.on(EVENT_TEST_PASS, function (test) {
var fmt =
indent() +
color('checkmark', ' ' + Base.symbols.ok) +
color('pass', ' %s');
var fmtVars = [test.title];
if (test.speed !== 'fast') {
fmt += color(test.speed, ' (%dms)');
fmtVars.push(test.duration);
}
if (test._currentRetry > 0) {
fmt += color('bright yellow', ' (retry x%d)');
fmtVars.push(test._currentRetry);
}
Base.consoleLog.apply(Base, [fmt].concat(fmtVars));
});
Included message within format so it can be passed into `Base.consoleLog` as one argument | ||
*/ | ||
var retryFmt = color( | ||
'bright yellow', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To really stick with the theme I think we should add a 'retries'
color here:
Lines 64 to 90 in 24560c1
/** | |
* Default color map. | |
*/ | |
exports.colors = { | |
pass: 90, | |
fail: 31, | |
'bright pass': 92, | |
'bright fail': 91, | |
'bright yellow': 93, | |
pending: 36, | |
suite: 0, | |
'error title': 0, | |
'error message': 31, | |
'error stack': 90, | |
checkmark: 32, | |
fast: 90, | |
medium: 33, | |
slow: 31, | |
green: 32, | |
light: 90, | |
'diff gutter': 90, | |
'diff added': 32, | |
'diff removed': 31, | |
'diff added inline': '30;42', | |
'diff removed inline': '30;41' | |
}; |
And possibly stick even more with the style of eg. durations and resolve to the color in base.js
:
Lines 369 to 377 in 24560c1
runner.on(EVENT_TEST_PASS, function (test) { | |
if (test.duration > test.slow()) { | |
test.speed = 'slow'; | |
} else if (test.duration > test.slow() / 2) { | |
test.speed = 'medium'; | |
} else { | |
test.speed = 'fast'; | |
} | |
}); |
And maybe do a similar style and have two levels: 'some retries'
/ 'many retries'
With only the latter resolving to bright yellow
@@ -67,20 +67,34 @@ function Spec(runner, options) { | |||
}); | |||
|
|||
runner.on(EVENT_TEST_PASS, function (test) { | |||
function logRetries() { | |||
if (test._currentRetry > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underscore in _currentRetry
is an old-school practice for indicating that its a private property, so we shouldn't rely on it here I think as its not intended for outside consumption.
If we want it to be exposed we should make it an explicit public property, like duration
, and probably name it retries
or retryCount
or such
We should use the public .currentRetry()
instead.
👋 Ping @CheadleCheadle, is this still something you have energy & time for? |
Closing out to free up the backing issue. @CheadleCheadle if you end up seeing this and having time again, no worries, feel free to re-open the PR or send a new one. If anybody else sees this message and there isn't a new PR, feel free to send one yourself. Just make sure to add a co-author attribution if it takes code from this PR. Cheers all! 🤎 |
PR Checklist
status: accepting prs
Overview
I've implemented a function called logRetries() which displays the number of the current retry along with the total number of retries, provided that the total number of retries is greater than zero. This output is formatted below the test title and highlighted in yellow as requested for ecosystem consistency. I would greatly appreciate any feedback on this addition!
Example: